Add to_tensor support for VideoEncoder #957
Conversation
meta-cla
bot
added
the
CLA Signed
NicolasHug
left a comment
NicolasHug
left a comment
There was a problem hiding this comment.
Thanks for the PR @Dan-Flores, nice work ! Left a few comments and suggestions but this looks great
| create_from_tensor, | ||
| encode_audio_to_file, | ||
| encode_video_to_file, | ||
| encode_video_to_tensor, |
There was a problem hiding this comment.
Let's add some more test:
- let's also parametrize
test_bad_inputover the method, when relevant - let's have a similar test to
torchcodec/test/test_encoders.py
Line 345 in b084768
These tests can be easier to write (and read!) when we have the public Python VideoEncoder class. So if you prefer this can be left as a TODO, but we should make sure to follow up on that.
There was a problem hiding this comment.
I'll add a similar test to this PR, hopefully it is fairly readable.
I may move some tests to test_encoders.py once I push a PR with the python API, since we really want to test the VideoEncoder entrypoint.
There was a problem hiding this comment.
Parametrizing test_bad_input without a VideoEncoder class is fairly messy. I'll move this test to test_encoders.py and clean it up. For now, I've added the case where a bad format is passed in.
There was a problem hiding this comment.
That sounds good, maybe we can just write a TODO to parametrize test_bad_input when we move it to test_encoders.py ?
test/test_ops.py
Outdated
| assert_close(s_frame, rt_frame, atol=atol, rtol=0) | ||
|
|
||
| @pytest.mark.parametrize( | ||
| "format", ("mov", "mp4", "avi", "mkv", "webm", "flv", "gif") |
There was a problem hiding this comment.
Let's mark the webm entry as slow (if it is!), see #968 (comment) for how to do that just for the webm entry, and not to the whole test
There was a problem hiding this comment.
Thanks for the pointer, I've added the mark. The webm tests are in fact very slow.
I suspect its because the encoding is done in GRB instead of YUV, thanks to avcodec_find_best_pix_fmt_of_list choosing the least lossy format, and the webm encoder supports GRB. We will probably want to change this for efficiency.
pytest --durations=25 -k "TestVideoEncoderOps"
============================================================ slowest 25 durations ============================================================
25.98s call test/test_ops.py::TestVideoEncoderOps::test_against_to_file[webm]
18.30s call test/test_ops.py::TestVideoEncoderOps::test_video_encoder_against_ffmpeg_cli[webm]
13.53s call test/test_ops.py::TestVideoEncoderOps::test_video_encoder_round_trip[to_tensor-webm]
13.33s call test/test_ops.py::TestVideoEncoderOps::test_video_encoder_round_trip[to_file-webm]
3.67s call test/test_ops.py::TestVideoEncoderOps::test_video_encoder_against_ffmpeg_cli[gif]
3.65s call test/test_ops.py::TestVideoEncoderOps::test_against_to_file[gif]
3.03s call test/test_ops.py::TestVideoEncoderOps::test_video_encoder_against_ffmpeg_cli[mkv]
2.78s call test/test_ops.py::TestVideoEncoderOps::test_video_encoder_against_ffmpeg_cli[mov]
2.71s call test/test_ops.py::TestVideoEncoderOps::test_video_encoder_against_ffmpeg_cli[mp4]
2.35s call test/test_ops.py::TestVideoEncoderOps::test_against_to_file[mkv]
2.26s call test/test_ops.py::TestVideoEncoderOps::test_against_to_file[mp4]
2.21s call test/test_ops.py::TestVideoEncoderOps::test_against_to_file[mov]
2.19s call test/test_ops.py::TestVideoEncoderOps::test_video_encoder_round_trip[to_tensor-mp4]
2.09s call test/test_ops.py::TestVideoEncoderOps::test_video_encoder_round_trip[to_file-mov]
There was a problem hiding this comment.
Thanks @Dan-Flores ! Left 3 non-blocking comments (one above, two below)
LGTM
| torch.testing.assert_close( | ||
| self.decode(encoded_file).data, self.decode(encoded_tensor).data | ||
| ) |
There was a problem hiding this comment.
These are uint8 so the default atol will be 0 anyway, but it's good to be explicit:
| torch.testing.assert_close( | |
| self.decode(encoded_file).data, self.decode(encoded_tensor).data | |
| ) | |
| torch.testing.assert_close( | |
| self.decode(encoded_file).data, self.decode(encoded_tensor).data, rtol=0, atol=0 | |
| ) |
| create_from_tensor, | ||
| encode_audio_to_file, | ||
| encode_video_to_file, | ||
| encode_video_to_tensor, |
There was a problem hiding this comment.
That sounds good, maybe we can just write a TODO to parametrize test_bad_input when we move it to test_encoders.py ?
Dan-Flores
force-pushed
the
video_to_tensor
branch
from
80d3999 to
231c68f
Compare
Co-authored-by: Daniel Flores <[email protected]>
3 participants
This PR enables encoding a video to tensor by adding a
VideoEncoderconstructor that accepts anAVIOContextHolderand fileformat. It is instantiated withAVIOToTensorContext.Testing:
test_video_encoder_round_tripis updated to testto_tensormethod.AVIOTensorContext.cppenable certain formats, such asaviandflvto be encoded correctly.test_video_encoder_round_trip, without the changes, they would raise an error when attempting to decode the encoded frames, indicating an incorrectly encoded video.maxfield allows encoders that write the header last (doing a backwards seek) to output a correctly encoded tensor.